net: NAD live-update non-migratable VM test#5212
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds DataVolume-backed VM spec support, reusable NAD patching, a non-migratable VM path, and a negative test that switches a secondary bridge NAD and verifies VLAN-specific connectivity outcomes. ChangesDataVolume-backed non-migratable VM NAD change test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot-6
requested review from
nirdothan,
rnetser and
servolkov
azhivovk
changed the title
openshift-virtualization-qe-bot-6
requested review from
RoniKishner,
frenzyfriday,
orelmisan and
yossisegev
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/vm/factory.py`:
- Around line 44-47: The condition on line 44 incorrectly suppresses the default
containerdisk insertion whenever any DataVolume is present, breaking the ability
to use DataVolumes as additional disks alongside a boot disk. Replace the logic
that checks for any DataVolume with a check that determines whether a boot disk
or containerdisk already exists. The containerdisk should be inserted by default
unless a boot disk is already present, allowing DataVolumes to coexist as
additional disks. Modify the condition in the containerdisk_storage function
call to verify the absence of an actual boot disk rather than the absence of any
DataVolume.
In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py`:
- Around line 244-248: The wait_for_vmi_condition_status call needs to be
anchored to the pre-patch resource state to ensure it validates the
RestartRequired condition is set as a result of the current patch operation, not
from a stale previous state. Capture the resourceVersion of
non_migratable_under_test_vm before calling patch_nad_references, then pass this
resourceVersion to the wait_for_vmi_condition_status call so it waits for a
condition transition that occurs after the patch, not a pre-existing condition
state.
In `@tests/network/libs/nad_ref.py`:
- Around line 27-40: The patch_nad_references function contains duplicate
network mutation logic that is already present in update_nad_references,
specifically the block that iterates through networks and updates the
multus.networkName based on the nad_name_by_net mapping. Extract this common
logic (the deepcopy, the for loop iterating through networks, and the
conditional update of network.multus.networkName) into a separate shared helper
function, then refactor both patch_nad_references and update_nad_references to
call this new helper function instead of duplicating the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70873300-8c28-46c1-a952-0a736d2105fb
📒 Files selected for processing (6)
libs/vm/factory.pylibs/vm/spec.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/network/libs/nad_ref.py
azhivovk
force-pushed
the
nad-ref-non-migratable
branch
from
524eef8 to
3ab53f3
Compare
|
@jpeimer Hi, please review the DV related changes |
|
Change: use |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/network/l2_bridge/nad_ref_change/lib_helpers.py`:
- Around line 161-186: The docstring in the helper that builds the Fedora bridge
VM spec is inaccurate and should be updated to match the implementation. Clarify
that the function uses data_volume_storage(data_volume.name) to reference an
already-created DataVolume by name and adds it via add_volume_disk to the VM
spec, rather than creating a dataVolumeTemplate or relying on the VM controller
to provision it. Keep the description aligned with _bridge_vm_spec,
data_volume_storage, and fedora_vm so readers understand the fixture depends on
an existing deployed DataVolume.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bdace48-2a5b-4bdf-825c-5bee5b90337a
📒 Files selected for processing (6)
libs/net/vmspec.pylibs/vm/spec.pylibs/vm/vm.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py
|
Change: edit docstring |
servolkov
left a comment
servolkov
left a comment
There was a problem hiding this comment.
The PR description looks outdated. In general LGTM, a few cosmetic comments:
|
Change: apply changes after CR
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/net/vmspec.py`:
- Around line 318-322: In vmspec’s network update flow, avoid masking a broken
template by defaulting `deepcopy(vm.template_spec.networks)` to an empty list
and then calling `vm.set_networks`; instead, fail fast if the template networks
are missing or invalid. Update the helper that rewrites networks so it only
proceeds when a matching Multus network exists for an entry in
`nad_name_by_net`, and if none of the NAD mappings can be applied, return an
error rather than letting the caller time out with `MigrationRequired` or
`RestartRequired`. Use the `vm.set_networks` path and the
`nad_name_by_net`/`network.multus` checks to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c0a9573-2923-4478-baa0-a4638224afe1
📒 Files selected for processing (6)
libs/net/vmspec.pylibs/vm/spec.pylibs/vm/vm.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py
|
Change: use DV template |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5285. Overlapping fileslibs/vm/vm.py |
|
Change: DV creation in non-migratable VM helper |
|
/verified Dual-stack BM with cnv-4.23: Test is not collected New test is passing when run without waiting for Regression due to helper |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5212"
# Ensure we are on the PR branch clone; derive merge-base against base branch.
base_ref=$(gh pr view "$pr" --repo "$repo" --json baseRefName --jq '.baseRefName')
head_sha=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
git fetch --quiet origin "$base_ref"
merge_base=$(git merge-base "origin/$base_ref" HEAD)
echo "BASE_REF=$base_ref"
echo "HEAD_SHA=$head_sha"
echo "MERGE_BASE=$merge_base"
echo
echo "=== Changed files ==="
git diff --name-only "$merge_base"...HEAD
echo
echo "=== First changed file and first changed line ==="
first_file=$(git diff --name-only "$merge_base"...HEAD | head -n1)
echo "FIRST_FILE=$first_file"
python - <<'PY' "$merge_base" "$first_file"
import re, subprocess, sys
base, path = sys.argv[1], sys.argv[2]
patch = subprocess.check_output(["git", "diff", "--unified=0", f"{base}...HEAD", "--", path], text=True)
line = None
for ln in patch.splitlines():
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', ln)
if m:
start = int(m.group(1))
# first changed content line on RIGHT side is the hunk start
line = start
break
if line is None:
raise SystemExit("Could not determine first changed line")
print(f"FIRST_LINE={line}")
PY
first_line=$(python - <<'PY' "$merge_base" "$first_file"
import re, subprocess, sys
base, path = sys.argv[1], sys.argv[2]
patch = subprocess.check_output(["git", "diff", "--unified=0", f"{base}...HEAD", "--", path], text=True)
for ln in patch.splitlines():
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', ln)
if m:
print(m.group(1))
break
PY
)
echo
echo "=== Modified symbols in libs and changed test helpers ==="
for f in libs/net/vmspec.py libs/vm/spec.py libs/vm/vm.py tests/network/l2_bridge/nad_ref_change/lib_helpers.py tests/network/l2_bridge/nad_ref_change/conftest.py tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --view expanded || true
echo "DIFF SYMBOL LINES:"
git diff --unified=0 "$merge_base"...HEAD -- "$f" | grep -nE '^[+-](def |class |`@pytest`\.fixture|[[:space:]]+[A-Za-z_][A-Za-z0-9_]*\s*:)' || true
echo
fi
done
echo
echo "=== Direct symbol searches in tests and conftest ==="
# Modified / introduced symbols based on summaries and expected diff
symbols=(
patch_nad_references
update_nad_references
wait_for_vmi_condition_status
verify_baseline_connectivity
non_migratable_bridge_vm
disk_and_volume_for_data_volume
DataVolumeRef
)
for sym in "${symbols[@]}"; do
echo "--- symbol: $sym"
rg -n -C2 --glob 'tests/**' --glob 'tests/**/conftest.py' --glob 'tests/**/*.py' "\\b${sym}\\b" tests || true
echo
done
echo
echo "=== Same-file callers / transitive expansion ==="
for f in libs/net/vmspec.py libs/vm/vm.py tests/network/l2_bridge/nad_ref_change/lib_helpers.py tests/network/l2_bridge/nad_ref_change/conftest.py; do
echo "--- $f"
rg -n -C2 'patch_nad_references|update_nad_references|verify_baseline_connectivity|non_migratable_bridge_vm|disk_and_volume_for_data_volume|DataVolumeRef' "$f" || true
echo
done
echo
echo "=== Changed file content slices ==="
sed -n '1,260p' tests/network/l2_bridge/nad_ref_change/conftest.py
echo
sed -n '1,260p' tests/network/l2_bridge/nad_ref_change/lib_helpers.py
echo
sed -n '1,320p' tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py
echo
sed -n '1,240p' libs/net/vmspec.py
echo
sed -n '1,220p' libs/vm/spec.py
echo
sed -n '1,220p' libs/vm/vm.py
echo
echo "=== Smoke and gating files ==="
echo "--- smoke"
rg -l '`@pytest`\.mark\.smoke' tests || true
echo "--- gating"
rg -l '`@pytest`\.mark\.gating' tests || true
echo
echo "=== Intersections / hierarchy checks for smoke and gating ==="
python - <<'PY'
from pathlib import Path
import subprocess, re
# Collect changed symbols and observed affected files manually from rg-based searches
def run(cmd):
return subprocess.check_output(cmd, text=True)
smoke_files = [p for p in run(["rg", "-l", r"`@pytest`\.mark\.smoke", "tests"]).splitlines() if p.strip()]
gating_files = [p for p in run(["rg", "-l", r"`@pytest`\.mark\.gating", "tests"]).splitlines() if p.strip()]
# Candidate affected files based on direct known usages from this PR
affected = set()
for pattern in [
r"\bverify_baseline_connectivity\b",
r"\bnon_migratable_bridge_vm\b",
r"\bpatch_nad_references\b",
]:
out = run(["rg", "-l", pattern, "tests"])
affected.update([p for p in out.splitlines() if p.strip()])
# Conftest hierarchy check for smoke files using modified symbols
symbols = ["patch_nad_references", "verify_baseline_connectivity", "non_migratable_bridge_vm"]
hier_hits = {}
for smoke in smoke_files:
p = Path(smoke).parent
checked = []
while True:
cf = p / "conftest.py"
if cf.exists():
txt = cf.read_text()
for s in symbols:
if re.search(rf"\b{s}\b", txt):
hier_hits.setdefault(smoke, []).append((str(cf), s))
checked.append(str(cf))
if str(p) == ".":
break
p = p.parent
print("SMOKE_FILES_COUNT", len(smoke_files))
print("GATING_FILES_COUNT", len(gating_files))
print("AFFECTED_DIRECT", sorted(affected))
print("SMOKE_HIER_HITS", hier_hits)
print("AFFECTED_GATING", sorted(set(gating_files) & affected))
PY
echo
echo "=== Build test plan body ==="
python - <<'PY' > /tmp/test_plan_comment.txt
from pathlib import Path
import subprocess, re, sys
def run(*cmd):
return subprocess.check_output(list(cmd), text=True)
# Verify actual dependency paths from repo content
conftest = Path("tests/network/l2_bridge/nad_ref_change/conftest.py").read_text()
helpers = Path("tests/network/l2_bridge/nad_ref_change/lib_helpers.py").read_text()
test_file = Path("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py").read_text()
# Determine smoke/gating usage
smoke_files = [p for p in run("rg", "-l", r"`@pytest`\.mark\.smoke", "tests").splitlines() if p.strip()]
gating_files = [p for p in run("rg", "-l", r"`@pytest`\.mark\.gating", "tests").splitlines() if p.strip()]
affected_tests = []
paths = []
# Direct affected tests from changed test file
if "test_non_migratable_vm_nad_change_not_applied" in test_file:
affected_tests.append("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied")
paths.append("test_non_migratable_vm_nad_change_not_applied → non_migratable_baseline_connectivity fixture → verify_baseline_connectivity() / non_migratable_under_test_vm fixture → non_migratable_bridge_vm() → libs.vm.vm.disk_and_volume_for_data_volume()")
# Same file changed tests using shared baseline helper
if "verify_baseline_connectivity" in conftest:
affected_tests.append("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_migratable_vm_nad_change_applied")
paths.append("test_migratable_vm_nad_change_applied → baseline_connectivity fixture → verify_baseline_connectivity()")
# patch_nad_references is only used in this changed test file
if "patch_nad_references" in test_file:
paths.append("test_non_migratable_vm_nad_change_not_applied → libs.net.vmspec.patch_nad_references()")
# Determine smoke impact: require verified path to smoke file or smoke conftest hierarchy usage
smoke_true = False
smoke_reason = ""
symbols = ["patch_nad_references", "verify_baseline_connectivity", "non_migratable_bridge_vm"]
for smoke in smoke_files:
p = Path(smoke).parent
while True:
cf = p / "conftest.py"
if cf.exists():
txt = cf.read_text()
for s in symbols:
if re.search(rf"\b{s}\b", txt):
smoke_true = True
smoke_reason = f"{smoke} → {cf} → {s}"
break
if smoke_true or str(p) == ".":
break
p = p.parent
if smoke_true:
break
# Determine gating impact: any gating-marked file in affected set
affected_set = set()
for pat in [r"\bverify_baseline_connectivity\b", r"\bnon_migratable_bridge_vm\b", r"\bpatch_nad_references\b"]:
out = run("rg", "-l", pat, "tests")
affected_set.update([p for p in out.splitlines() if p.strip()])
gating_hits = sorted(set(gating_files) & affected_set)
gating_true = bool(gating_hits)
gating_reason = ""
if gating_true:
# build specific path for first gating hit if available
g = gating_hits[0]
if g == "tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py":
gating_reason = "tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied (gating) → non_migratable_baseline_connectivity / non_migratable_under_test_vm → verify_baseline_connectivity() / non_migratable_bridge_vm() / patch_nad_references()"
else:
gating_reason = g
comment = []
comment.append("**Test Execution Plan**")
comment.append("")
comment.append(f"- **Run smoke tests:** {'True' if smoke_true else 'False'}" + (f" — {smoke_reason}" if smoke_true else " — no verified dependency path from any smoke-marked test or parent `conftest.py` to the modified symbols."))
comment.append(f"- **Run gating tests:** {'True' if gating_true else 'False'}" + (f" — {gating_reason}" if gating_true else ""))
comment.append("")
comment.append("**Affected tests to run**")
for t in affected_tests:
marker = " (gating)" if "non_migratable" in t else ""
comment.append(f"- `{t}`{marker}")
comment.append("")
comment.append("**Real tests (cluster required)**")
comment.append("Error path (the fix):")
comment.append("`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`")
comment.append("Expected: the VM NAD patch does not apply live; connectivity stays on VLAN-A and not VLAN-B. When KubeVirt wires `RestartRequired`, re-enable the wait and expect that condition before connectivity checks.")
comment.append("")
comment.append("Happy path (regression):")
comment.append("`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_migratable_vm_nad_change_applied`")
comment.append("Expected: the normal migratable NAD update path still applies live and connectivity switches as expected.")
print("\n".join(comment))
PY
cat /tmp/test_plan_comment.txt
echo
echo "=== Post inline review comment ==="
# Try submit as a single COMMENT review with one inline comment.
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/reviews" \
-f body='' \
-f event='COMMENT' \
-f commit_id="$head_sha" \
-f "comments[0][path]=$first_file" \
-F "comments[0][line]=$first_line" \
-f "comments[0][side]=RIGHT" \
-F "comments[0][body]=`@openshift-virtualization-qe-bot-3` $(cat /tmp/test_plan_comment.txt)" \
>/tmp/review_response.json || {
echo "Inline review creation failed, retrying with issue comment fallback" >&2
gh pr comment "$pr" --repo "$repo" --body "`@openshift-virtualization-qe-bot-3` $(cat /tmp/test_plan_comment.txt)"
}
echo "DONE"Length of output: 85114 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5212"
head_sha=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
first_file="libs/net/vmspec.py"
first_line="309"
echo "=== Inspect direct callers and markers for localnet NAD ref change ==="
sed -n '1,260p' tests/network/localnet/nad_ref_change/test_nad_ref_change.py
echo
echo "--- smoke marker in affected files"
rg -n '`@pytest`\.mark\.smoke|`@pytest`\.mark\.gating' \
tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py \
tests/network/localnet/nad_ref_change/test_nad_ref_change.py || true
cat > /tmp/test_plan_comment.txt <<'EOF'
**Test Execution Plan**
- **Run smoke tests:** False — no verified dependency path from any smoke-marked test or parent `conftest.py` to the modified symbols in `libs/net/vmspec.py`, `libs/vm/spec.py`, `libs/vm/vm.py`, or the changed NAD-ref-change helpers/fixtures.
- **Run gating tests:** False — no gating-marked test file appears in the affected set traced from `patch_nad_references()`, `update_nad_references()`, `verify_baseline_connectivity()`, `non_migratable_bridge_vm()`, and `disk_and_volume_for_data_volume()`.
**Affected tests to run**
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_vm_state_iface_info_preserved`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_connectivity`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_two_networks`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Dependency paths:
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied` → `non_migratable_baseline_connectivity` / `non_migratable_under_test_vm` fixtures in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` / `non_migratable_bridge_vm()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py` → `disk_and_volume_for_data_volume()` in `libs/vm/vm.py`, plus direct call to `patch_nad_references()` in `libs/net/vmspec.py`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::*` → `baseline_connectivity` fixture in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py`; the tests also call `update_nad_references()` in `libs/net/vmspec.py`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py` → direct call(s) to `update_nad_references()` in `libs/net/vmspec.py`, which now delegate to `patch_nad_references()`
**Real tests (cluster required)**
Error path (the fix):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
Expected: the VM NAD patch does not apply live; connectivity stays on VLAN-A and not VLAN-B. Once KubeVirt wires `RestartRequired`, the condition wait should also pass before the connectivity assertions.
Happy path (regression):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange`
Expected: the normal migratable NAD update path still applies live and connectivity follows the updated NAD references.
Additional regression for the refactored shared helper:
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Expected: the localnet NAD reference update path still applies live after `update_nad_references()` was refactored to call `patch_nad_references()`.
EOF
python - <<'PY'
import json, pathlib
body = pathlib.Path("/tmp/test_plan_comment.txt").read_text()
payload = {
"body": "",
"event": "COMMENT",
"commit_id": "440085587c13fb65d210f9684f3d781d8e5841fd",
"comments": [
{
"path": "libs/net/vmspec.py",
"line": 309,
"side": "RIGHT",
"body": body,
}
],
}
pathlib.Path("/tmp/review_payload.json").write_text(json.dumps(payload))
print(body)
PY
echo
echo "=== Post inline review comment via raw JSON ==="
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/reviews" \
--input /tmp/review_payload.jsonLength of output: 12236 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5212"
cat > /tmp/test_plan_comment.txt <<'EOF'
**Test Execution Plan**
- **Run smoke tests:** False — no verified dependency path from any smoke-marked test or parent `conftest.py` to the modified symbols in `libs/net/vmspec.py`, `libs/vm/spec.py`, `libs/vm/vm.py`, or the changed NAD-ref-change helpers/fixtures.
- **Run gating tests:** False — no gating-marked test file appears in the affected set traced from `patch_nad_references()`, `update_nad_references()`, `verify_baseline_connectivity()`, `non_migratable_bridge_vm()`, and `disk_and_volume_for_data_volume()`.
**Affected tests to run**
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_vm_state_iface_info_preserved`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_connectivity`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_two_networks`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Dependency paths:
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied` → `non_migratable_baseline_connectivity` / `non_migratable_under_test_vm` fixtures in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` / `non_migratable_bridge_vm()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py` → `disk_and_volume_for_data_volume()` in `libs/vm/vm.py`, plus direct call to `patch_nad_references()` in `libs/net/vmspec.py`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::*` → `baseline_connectivity` fixture in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py`; the tests also call `update_nad_references()` in `libs/net/vmspec.py`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py` → direct call(s) to `update_nad_references()` in `libs/net/vmspec.py`, which now delegate to `patch_nad_references()`
**Real tests (cluster required)**
Error path (the fix):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
Expected: the VM NAD patch does not apply live; connectivity stays on VLAN-A and not VLAN-B. Once KubeVirt wires `RestartRequired`, the condition wait should also pass before the connectivity assertions.
Happy path (regression):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange`
Expected: the normal migratable NAD update path still applies live and connectivity follows the updated NAD references.
Additional regression for the refactored shared helper:
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Expected: the localnet NAD reference update path still applies live after `update_nad_references()` was refactored to call `patch_nad_references()`.
EOF
python - <<'PY'
import json, pathlib
payload = {"body": pathlib.Path("/tmp/test_plan_comment.txt").read_text()}
pathlib.Path("/tmp/issue_comment_payload.json").write_text(json.dumps(payload))
PY
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/issues/$pr/comments" \
--input /tmp/issue_comment_payload.jsonLength of output: 4498
CRITICAL: I could not post the required inline review comment because the GitHub integration for this run does not have write access to the PR. All three write paths failed with
That rule exists to ensure the plan is attached to the actual diff, but here the permission boundary is the blocker, not the analysis. Here is the exact test execution plan that should be posted on Test Execution Plan
Affected tests to run
Dependency paths:
Real tests (cluster required) Happy path (regression): Additional regression for the refactored shared helper: |
|
@coderabbitai I ran the new test without the condition waiting to make sure it'll pass once we unblock it |
|
Tip For best results, initiate chat on the files or code changes.
The linked verification comment covers the regression path I was concerned about as well. 🐇 |
The VM spec lacked typed support for volumes backed by a DataVolume, forcing callers to manipulate vm.body["spec"] directly and bypass the typed spec layer. DataVolumeRef maps the KubeVirt dataVolume volume source to the same typed pattern used by ContainerDisk and CloudInitNoCloud. patch_nad_references splits the patch step from update_nad_references so callers that cannot trigger a migration can patch the NAD reference and wait on a different condition separately. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
A NAD reference change on a non-live-migratable VM cannot be applied without a restart. The expected behavior is that KubeVirt sets a RestartRequired condition and the VM retains connectivity to the original VLAN until restarted. A RWO DataVolume is embedded as a dataVolumeTemplate so the VM owns and creates it. Each DV is bound to exactly one VM; creating it inside the helper ensures the DV is scheduled on the same node as the VM so the RWO PVC can be claimed. Quarantined via CNV-87878: the RestartRequired condition is not yet wired up in KubeVirt, so the wait step will remain skipped until the fix lands and the quarantine is lifted. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Change: inline disk and volume helper |
|
/verified |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5212"
base_ref=$(gh pr view "$pr" --repo "$repo" --json baseRefName --jq '.baseRefName')
head_sha=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
git fetch --quiet origin "$base_ref"
merge_base=$(git merge-base "origin/$base_ref" HEAD)
echo "BASE_REF=$base_ref"
echo "HEAD_SHA=$head_sha"
echo "MERGE_BASE=$merge_base"
echo
echo "=== Changed files ==="
git diff --name-only "$merge_base"...HEAD
first_file=$(git diff --name-only "$merge_base"...HEAD | head -n1)
echo
echo "FIRST_FILE=$first_file"
first_line=$(python - <<'PY' "$merge_base" "$first_file"
import re, subprocess, sys
base, path = sys.argv[1], sys.argv[2]
patch = subprocess.check_output(["git", "diff", "--unified=0", f"{base}...HEAD", "--", path], text=True)
for ln in patch.splitlines():
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', ln)
if m:
print(m.group(1))
break
else:
raise SystemExit("Could not determine first changed line")
PY
)
echo "FIRST_LINE=$first_line"
echo
echo "=== Outline changed candidate files ==="
for f in \
libs/net/vmspec.py \
libs/vm/spec.py \
libs/vm/vm.py \
tests/network/l2_bridge/nad_ref_change/lib_helpers.py \
tests/network/l2_bridge/nad_ref_change/conftest.py \
tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py \
tests/network/localnet/nad_ref_change/test_nad_ref_change.py
do
if [ -f "$f" ]; then
echo "--- outline: $f"
ast-grep outline "$f" --view expanded || true
echo
fi
done
echo
echo "=== Diff hunks for changed files ==="
for f in $(git diff --name-only "$merge_base"...HEAD); do
echo "--- diff: $f"
git diff --unified=0 "$merge_base"...HEAD -- "$f" || true
echo
done
echo
echo "=== Modified defs/classes in libs and shared helpers ==="
for f in libs/net/vmspec.py libs/vm/spec.py libs/vm/vm.py tests/network/l2_bridge/nad_ref_change/lib_helpers.py tests/network/l2_bridge/nad_ref_change/conftest.py tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py; do
if [ -f "$f" ]; then
echo "--- symbols from $f"
git diff --unified=0 "$merge_base"...HEAD -- "$f" | grep -nE '^[+-](def |class )' || true
echo
fi
done
echo
echo "=== Current file slices ==="
sed -n '1,260p' libs/net/vmspec.py
echo
sed -n '1,260p' libs/vm/spec.py
echo
sed -n '1,260p' libs/vm/vm.py
echo
sed -n '1,320p' tests/network/l2_bridge/nad_ref_change/lib_helpers.py
echo
sed -n '1,260p' tests/network/l2_bridge/nad_ref_change/conftest.py
echo
sed -n '1,360p' tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py
echo
sed -n '1,260p' tests/network/localnet/nad_ref_change/test_nad_ref_change.py
echo
echo "=== Direct caller search in tests/conftest ==="
for sym in \
patch_nad_references \
update_nad_references \
wait_for_vmi_condition_status \
verify_baseline_connectivity \
non_migratable_bridge_vm \
DataVolumeRef
do
echo "--- symbol: $sym"
rg -n -C2 "\\b${sym}\\b" tests || true
echo
done
echo
echo "=== Same-file/transitive caller search ==="
for f in libs/net/vmspec.py libs/vm/vm.py tests/network/l2_bridge/nad_ref_change/lib_helpers.py tests/network/l2_bridge/nad_ref_change/conftest.py; do
echo "--- $f"
rg -n -C2 'patch_nad_references|update_nad_references|verify_baseline_connectivity|non_migratable_bridge_vm|DataVolumeRef' "$f" || true
echo
done
echo
echo "=== Smoke and gating files ==="
echo "--- smoke files"
rg -l '`@pytest`\.mark\.smoke' tests || true
echo "--- gating files"
rg -l '`@pytest`\.mark\.gating' tests || true
echo
echo "=== Build precise test plan ==="
python - <<'PY' > /tmp/test_plan_comment.txt
from pathlib import Path
import subprocess, re
def run(*cmd):
return subprocess.check_output(list(cmd), text=True)
changed_files = set(run("git", "diff", "--name-only", "$(git merge-base origin/main HEAD)...HEAD"))
# Read relevant files
l2_test = Path("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py").read_text()
l2_conftest = Path("tests/network/l2_bridge/nad_ref_change/conftest.py").read_text()
l2_helpers = Path("tests/network/l2_bridge/nad_ref_change/lib_helpers.py").read_text()
localnet_test = Path("tests/network/localnet/nad_ref_change/test_nad_ref_change.py").read_text()
vmspec = Path("libs/net/vmspec.py").read_text()
vm_py = Path("libs/vm/vm.py").read_text()
spec_py = Path("libs/vm/spec.py").read_text()
# Determine affected tests by verified paths only
affected = []
paths = []
# L2 existing class tests: use baseline_connectivity + update_nad_references
for name in [
"test_vm_state_iface_info_preserved",
"test_connectivity",
"test_two_networks",
]:
if name in l2_test and "verify_baseline_connectivity" in l2_conftest and "update_nad_references" in l2_test:
affected.append(f"tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::{name}")
paths.append("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::* → baseline_connectivity fixture in tests/network/l2_bridge/nad_ref_change/conftest.py → verify_baseline_connectivity() in tests/network/l2_bridge/nad_ref_change/lib_helpers.py; these tests also call update_nad_references() in libs/net/vmspec.py, which now delegates patching through patch_nad_references().")
# New non-migratable test
if "test_non_migratable_vm_nad_change_not_applied" in l2_test:
affected.append("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied")
paths.append("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied → non_migratable_baseline_connectivity / non_migratable_under_test_vm fixtures in tests/network/l2_bridge/nad_ref_change/conftest.py → verify_baseline_connectivity() / non_migratable_bridge_vm() in tests/network/l2_bridge/nad_ref_change/lib_helpers.py → VM spec/DataVolume wiring in libs/vm/spec.py and libs/vm/vm.py; the test also calls patch_nad_references() in libs/net/vmspec.py directly.")
# Localnet regression from update_nad_references refactor
if "update_nad_references" in localnet_test:
affected.append("tests/network/localnet/nad_ref_change/test_nad_ref_change.py")
paths.append("tests/network/localnet/nad_ref_change/test_nad_ref_change.py → direct call(s) to update_nad_references() in libs/net/vmspec.py → patch_nad_references().")
# Smoke impact: verified path only
smoke_files = [p for p in run("rg", "-l", "`@pytest`\\.mark\\.smoke", "tests").splitlines() if p.strip()]
gating_files = [p for p in run("rg", "-l", "`@pytest`\\.mark\\.gating", "tests").splitlines() if p.strip()]
modified_symbols = ["patch_nad_references", "update_nad_references", "verify_baseline_connectivity", "non_migratable_bridge_vm", "DataVolumeRef"]
affected_set = set()
for sym in modified_symbols:
out = run("rg", "-l", rf"\b{sym}\b", "tests")
for p in out.splitlines():
if p.strip():
affected_set.add(p.strip())
smoke_true = False
smoke_reason = "no verified dependency path from any smoke-marked test or parent `conftest.py` to the modified symbols."
for smoke in smoke_files:
if smoke in affected_set:
smoke_true = True
smoke_reason = f"{smoke} directly references a modified symbol."
break
p = Path(smoke).parent
while True:
cf = p / "conftest.py"
if cf.exists():
txt = cf.read_text()
for sym in modified_symbols:
if re.search(rf"\b{sym}\b", txt):
smoke_true = True
smoke_reason = f"{smoke} → {cf} → {sym}"
break
if smoke_true or str(p) == ".":
break
p = p.parent
gating_hits = sorted(set(gating_files) & affected_set)
gating_true = bool(gating_hits)
gating_reason = "no gating-marked test file appears in the affected set traced from the modified symbols."
if gating_true:
g = gating_hits[0]
if g == "tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py":
gating_reason = "tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied (gating) → non_migratable_baseline_connectivity / non_migratable_under_test_vm → verify_baseline_connectivity() / non_migratable_bridge_vm() / patch_nad_references()."
else:
gating_reason = f"{g} references a modified symbol."
comment = []
comment.append("**Test Execution Plan**")
comment.append("")
comment.append(f"- **Run smoke tests:** {'True' if smoke_true else 'False'} — {smoke_reason}")
comment.append(f"- **Run gating tests:** {'True' if gating_true else 'False'} — {gating_reason}")
comment.append("")
comment.append("**Affected tests to run**")
for item in affected:
marker = " (gating)" if item.endswith("::test_non_migratable_vm_nad_change_not_applied") else ""
comment.append(f"- `{item}`{marker}")
comment.append("")
comment.append("Dependency paths:")
for p in paths:
comment.append(f"- {p}")
comment.append("")
comment.append("**Real tests (cluster required)**")
comment.append("Error path (the fix):")
comment.append("`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`")
comment.append("Expected: the VM NAD patch does not apply live; connectivity stays on VLAN-A and not VLAN-B. Once KubeVirt wires `RestartRequired`, restore the wait and expect that condition before the connectivity assertions.")
comment.append("")
comment.append("Happy path (regression):")
comment.append("`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange`")
comment.append("Expected: the normal migratable NAD update path still applies live and connectivity follows the updated NAD references.")
comment.append("")
comment.append("Additional regression for the shared `update_nad_references()` refactor:")
comment.append("`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/localnet/nad_ref_change/test_nad_ref_change.py`")
comment.append("Expected: the localnet NAD reference update path still applies live after `update_nad_references()` was refactored to call `patch_nad_references()`.")
print("\n".join(comment))
PY
cat /tmp/test_plan_comment.txt
echo
echo "=== Post inline review comment (retry + fallback) ==="
python - <<'PY' "$repo" "$pr" "$head_sha" "$first_file" "$first_line"
import json, pathlib, subprocess, sys, time
repo, pr, head_sha, first_file, first_line = sys.argv[1:]
body = pathlib.Path("/tmp/test_plan_comment.txt").read_text()
review_payload = {
"body": "",
"event": "COMMENT",
"commit_id": head_sha,
"comments": [
{
"path": first_file,
"line": int(first_line),
"side": "RIGHT",
"body": body,
}
],
}
path = pathlib.Path("/tmp/review_payload.json")
path.write_text(json.dumps(review_payload))
def run(cmd):
print("+", " ".join(cmd))
return subprocess.run(cmd, text=True, capture_output=True)
# Two inline attempts
for attempt in (1, 2):
print(f"INLINE_ATTEMPT={attempt}")
res = run([
"gh", "api", "--method", "POST",
"-H", "Accept: application/vnd.github+json",
f"/repos/{repo}/pulls/{pr}/reviews",
"--input", str(path),
])
print(res.stdout)
print(res.stderr)
if res.returncode == 0:
sys.exit(0)
time.sleep(2)
# Fallback issue comment
issue_payload = {"body": body}
issue_path = pathlib.Path("/tmp/issue_comment_payload.json")
issue_path.write_text(json.dumps(issue_payload))
print("FALLBACK=issue_comment")
res = run([
"gh", "api", "--method", "POST",
"-H", "Accept: application/vnd.github+json",
f"/repos/{repo}/issues/{pr}/comments",
"--input", str(issue_path),
])
print(res.stdout)
print(res.stderr)
sys.exit(res.returncode)
PYLength of output: 99887 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5212"
first_file="libs/net/vmspec.py"
first_line="309"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
cat > /tmp/test_plan_comment.txt <<'EOF'
**Test Execution Plan**
- **Run smoke tests:** False — no verified dependency path from any smoke-marked test or parent `conftest.py` to the modified symbols.
- **Run gating tests:** False — no gating-marked test file appears in the affected set traced from `patch_nad_references()`, `update_nad_references()`, `verify_baseline_connectivity()`, `non_migratable_bridge_vm()`, and `DataVolumeRef`.
**Affected tests to run**
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_vm_state_iface_info_preserved`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_connectivity`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_two_networks`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Dependency paths:
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::*` → `baseline_connectivity` fixture in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py`; these tests also call `update_nad_references()` in `libs/net/vmspec.py`, which delegates patching through `patch_nad_references()`.
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied` → `non_migratable_baseline_connectivity` / `non_migratable_under_test_vm` fixtures in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` / `non_migratable_bridge_vm()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py` → VM spec/DataVolume wiring in `libs/vm/spec.py`; the test also calls `patch_nad_references()` in `libs/net/vmspec.py` directly.
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py` → direct call(s) to `update_nad_references()` in `libs/net/vmspec.py` → `patch_nad_references()`.
**Real tests (cluster required)**
Error path (the fix):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
Expected: the VM NAD patch does not apply live; connectivity stays on VLAN-A and not VLAN-B. Once KubeVirt wires `RestartRequired`, restore the wait and expect that condition before the connectivity assertions.
Happy path (regression):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange`
Expected: the normal migratable NAD update path still applies live and connectivity follows the updated NAD references.
Additional regression for the shared `update_nad_references()` refactor:
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Expected: the localnet NAD reference update path still applies live after `update_nad_references()` was refactored to call `patch_nad_references()`.
EOF
python - <<'PY'
import json, pathlib
body = pathlib.Path("/tmp/test_plan_comment.txt").read_text()
review_payload = {
"body": "",
"event": "COMMENT",
"commit_id": pathlib.Path("/tmp/head_sha.txt").read_text().strip() if pathlib.Path("/tmp/head_sha.txt").exists() else None,
"comments": []
}
print(body)
PY
echo "$head_sha" > /tmp/head_sha.txt
python - <<'PY' "$head_sha" "$first_file" "$first_line"
import json, pathlib, sys
head_sha, first_file, first_line = sys.argv[1:]
body = pathlib.Path("/tmp/test_plan_comment.txt").read_text()
payload = {
"body": "",
"event": "COMMENT",
"commit_id": head_sha,
"comments": [
{
"path": first_file,
"line": int(first_line),
"side": "RIGHT",
"body": body,
}
],
}
pathlib.Path("/tmp/review_payload.json").write_text(json.dumps(payload))
pathlib.Path("/tmp/issue_comment_payload.json").write_text(json.dumps({"body": body}))
PY
echo
echo "=== Inline review attempt 1 ==="
set +e
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/reviews" \
--input /tmp/review_payload.json
status1=$?
set -e
echo "INLINE_STATUS_1=$status1"
if [ "$status1" -ne 0 ]; then
echo
echo "=== Inline review attempt 2 ==="
set +e
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/reviews" \
--input /tmp/review_payload.json
status2=$?
set -e
echo "INLINE_STATUS_2=$status2"
if [ "$status2" -ne 0 ]; then
echo
echo "=== Fallback issue comment ==="
set +e
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/issues/$pr/comments" \
--input /tmp/issue_comment_payload.json
status3=$?
set -e
echo "ISSUE_COMMENT_STATUS=$status3"
exit "$status3"
fi
fiLength of output: 10130
CRITICAL: I could not post the required inline review comment because this run's GitHub integration does not have write access to the PR. All required write paths failed with the same error:
The analysis is complete. The exact comment that should be posted on Test Execution Plan
Affected tests to run
Dependency paths:
Real tests (cluster required) Happy path (regression): Additional regression for the shared |
|
/lgtm |
frenzyfriday
left a comment
frenzyfriday
left a comment
There was a problem hiding this comment.
lgtm from the NAD/nw side, thanks!
|
/approve |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
11 participants
What this PR does / why we need it:
Adds
test_non_migratable_vm_nad_change_not_applied: verifies that updating the NAD reference on a non-migratable VM does not apply live and the VM reports aRestartRequiredcondition and retains connectivity to the original VLAN-A.To support this, the PR also:
DataVolumeRefvolume support anddataVolumeTemplates; adds anon_migratable_bridge_vmfactory that embeds an RWO DataVolume as adataVolumeTemplateso the VM ownsit and the RWO PVC prevents live migration
patch_nad_referencesfromupdate_nad_referencesas a patch-only variant for non-migratable VMs, where the expected post-change condition isRestartRequiredrather than live migrationWhich issue(s) this PR fixes: -
Special notes for reviewer:
The test is quarantined via @pytest.mark.jira("CNV-87878", run=False): the
RestartRequiredcondition is not yet wired up in KubeVirt.jira-ticket: https://redhat.atlassian.net/browse/CNV-89745
Summary by CodeRabbit
Summary
New Features
Bug Fixes
Tests